Handle SSLZeroReturnError exceptions in stdlib TLS adapter
#518
Conversation
toppk
force-pushed
the
main
branch
2 times, most recently
from
4995f4e to
9f17e91
Compare
toppk
changed the title
|
not sure why it doesn't like my commit message. |
Don't worry, that platisd/bad-commit-message-blocker check is broken, I haven't gotten to fixing it. But the one in pre-commit (see https://results.pre-commit.ci/run/github/16620627/1663816002.9KC-J1hTS36wzU6ARo-C4w) where there's two flake8 violations introduced is a legit failure. The complexity has been increased past the acceptable limit and so it needs to be simplified for the linting to go green. |
cheroot/ssl/builtin.py
Outdated
| # This is almost certainly due to the cherrypy engine | ||
| # 'pinging' the socket to assert it's connectable; | ||
| # the 'ping' isn't SSL. |
There was a problem hiding this comment.
You said that you've disabled the Checker and it's still happening. Pretty sure it's unrelated, then.
It must be this https://github.com/cherrypy/cheroot/blob/3f9b1cb/cheroot/ssl/builtin.py#L55-L83.
By the way, maybe the suppress() in that helper function needs to be extended too.
There was a problem hiding this comment.
It isn't the checker, but cherrypy has a call to a module called portend which is used to move the lifecycle of the service to the active state. This is a simple responds to TCP port check. So it is a check but not the checker...
There was a problem hiding this comment.
Oh, that's good to know. That would be useful to mention.
cheroot/ssl/builtin.py
Outdated
| # This is almost certainly due to the cherrypy engine | ||
| # 'pinging' the socket to assert it's connectable; | ||
| # the 'ping' isn't SSL. | ||
| return EMPTY_RESULT | ||
| except ssl.SSLError as ex: | ||
| if ex.errno == ssl.SSL_ERROR_EOF: |
There was a problem hiding this comment.
I wonder if we could declare an internal CherootSSLZeroReturnError in cheroot._compat, and then wrap the TLS calls to re-raise both ssl.SSLZeroReturnError and ssl.SSLError with ssl.SSL_ERROR_EOF as this internal exception. This would allow having a cleaner structural block for handling two incarnations of the same situation as one thing without having to care about two copy-pasted blocks are handled the same way.
There was a problem hiding this comment.
Oh, I see, we already have errors.NoSSLError so maybe not _compat but errors would be a better place for the exception.
There was a problem hiding this comment.
And maybe all these branchy cases could be treated the same with a shorter and better readable error handling on this layer of abstraction.
There was a problem hiding this comment.
in doing a full review of how this exception is handled, and comparing to the pyopenssl implementation, I would say my opinion is that we should change this entire exception code to just swallow the exception and return EMPTY_RESULT, except for the http-on-https port condition.
Something like this.
try:
s = self.context.wrap_socket(
sock,
do_handshake_on_connect=True,
server_side=True,
)
return s, self.get_environ(s)
except ssl.SSLError as ex:
if ex.errno == ssl.SSL_ERROR_SSL:
if _assert_ssl_exc_contains(ex, "http request"):
# The client is speaking HTTP to an HTTPS server.
raise errors.NoSSLError
except (generic_socket_error, ssl.SSLZeroReturnError):
pass
return EMPTY_RESULTIf we ever wanted to log these bad sockets, we can add to this, but right now we dissect a bunch of exceptions all for it to be ignored anyway (sometimes the stack trace is printed, but other than that it is ignored).
I would also get rid of the except Exception: in server.serve() as there shouldn't be any unknown/unexpected exceptions bubbling up there, the only unknown/unexpected exceptions should be coming from user code, and the workers are in different threads.
There was a problem hiding this comment.
My understanding is that the reason this is more complicated than pyopenssl is that pyopenssl does some processing internally and turns weird cases into specific exceptions. I'd love the same to happen with the ssl implementation. But it'd have to happen on our side since stdlib doesn't do this. And even if it did, we'd need to have something for compatibility for years.
I would also get rid of the
except Exception:inserver.serve()as there shouldn't be any unknown/unexpected exceptions bubbling up there, the only unknown/unexpected exceptions should be coming from user code, and the workers are in different threads.
Agreed. Although, this deserves a separate discussion/PR.
|
Hey @mxii-ca, you've provided a lot of invaluable input on TLS in the past. Would you mind checking this patch? Am I right that it's related to https://github.com/cherrypy/cheroot/blob/3f9b1cb/cheroot/ssl/builtin.py#L55-L83? |
|
I've added a second commit that attempts simplification of the code. The big difference is that the earlier code would reraise some exceptions that wrap_socket raised, and this will just ignore all of them except for that only exception that cheroot handles (http-over-https aka errors.NoSSLError). If this approach is satisfactory I can squash the commits for a cleaner commit log. I will also remove _compat.IS_ABOVE_OPENSSL10 and related code. |
|
This pull request introduces 2 alerts when merging 6c98cef into 3915f35 - view on LGTM.com new alerts:
|
thank you bot!
yes, I didn't notice that, this code can get removed as well.
if this approach is accepted, I will remove this code |
Well, it may cause hard-to-debug states if the exceptions are just ignored silently. The currently listed errors were carefully curated, and I'm not sure if it's reasonable to throw it all away without a mechanism for inspection. It'd be good to at least have some logging there, I'm not sure. I see your point that most of the exceptions would need to be ignored anyway, but I foresee difficulties for the end-users attempting to figure out what they've set up wrong in the TLS configuration, and it tends to be nontrivial when it comes to understanding crypto stuff like ciphers. How would one see that a client is sending requests with non-matching ciphers enabled, for example? By the way, I'm not sure if you misunderstood my initial idea — I was hoping that the call to |
|
This pull request introduces 1 alert when merging 2a9e795 into 03cc9a8 - view on LGTM.com new alerts:
|
I can see that the curation was a hard fought for list, but when I looked at pyopenssl backend I chuckled a bit. As for logging, it is a no win situation, as either it will annoy some people or not annoy others. For an internet facing service, logs will get litter with the results of random port scans, for some development/production scenarios it may be desirable, but that might also include things that are on the curated list.
of course, tls communicates back to the client failure, so silently ignoring on the server side is not removing all information. The two issues I have with current approach,
Another thing while the current blacklist approach of generating a stack trace on all unknown exceptions is very nice to send users your way (as it did me), it has the obvious downside where we are chasing new exceptions categories, and it is not obvious what do. An actual whitelist of exceptions we want to log, and a facility for users to turn on/off said logging may be useful.
Yeah, I was thinking about extending context, and then wrap socket can produce better errors, or a solution like the with supress() that exists elsewhere, but I think those would just obfuscate the execution from the code. The wrap() function is the clearest way to express how we want to handle the wrap exceptions, and doesn't do much else. It would be cool if except got some of the PEP 634/Structural Pattern Matching features, but maybe this use case is just going to look like a bunch of duct tape no matter what (bad exceptions being raised, supporting multiple python runtimes). I may send something to python-ideas anyway. |
|
This pull request introduces 1 alert when merging 5ede676 into 6ce6e1e - view on LGTM.com new alerts:
|
Yeah. Although, I wouldn't bundle any behavior changes in this PR anyway — I think it should just contain a bugfix with minimum changes. And any refactoring needs to go elsewhere.
How about a method? def _wrap_context_socket_with_consistent_errors(self, sock):
try:
return self.context.wrap_socket(
sock, do_handshake_on_connect=True, server_side=True,
)
except ...:
raise CherootTLSEOL
except ...:
raise CherootTLSEOL
def wrap(self, sock)
try:
return self._wrap_context_socket_with_consistent_errors(sock)
except CherootTLSEOL:
...
except ...:
...
That would be a no-go for us for another 3-4 years because of compat requirements. Libs don't have the same luxury that apps do. |
|
This pull request introduces 1 alert when merging 24e692a into e0dd9cb - view on LGTM.com new alerts:
|
webknjaz
force-pushed
the
main
branch
2 times, most recently
from
202fe0b to
7a4712a
Compare
|
This pull request introduces 1 alert when merging 7a4712a into 85b8dc9 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4c5aba6 into ec51fbb - view on LGTM.com new alerts:
|
|
i'll take a look at your last recommendation and update this pr. not sure it will pass the complexity review, but we'll see. |
|
Ref: #346 |
|
FWIW, I can confirm this fixes #517 for me. |
|
Could we please get this merged? @toppk Are you still planning on pushing this PR to completion? |
I am willing to, but I'm having a hard time incorporating the feedback into a new design. I will revisit this week and create something so there can be some further discussion/action on this. |
|
Do you need any help with this? |
This comment was marked as off-topic.
This comment was marked as off-topic.
webknjaz
changed the title
SSLZeroReturnError exceptions in stdlib TLS adapter
@toppk could you clarify why you're talking about |
because its one and the same, openssl changed things and cpython reacted and such, |
Oh, I didn't realize, thanks! |
Python 3.8 introduced a different exception for zero byte tcp connections. These connections are generated by cherrypy on startup, and so the exception is not displayed as it is expected. Without this fix an exception is displayed although it is harmless. This treats the new exception just like it was treated under the errno exception.
webknjaz
force-pushed
the
main
branch
2 times, most recently
from
95ae5ce to
9dca4a9
Compare
Currently, sockets with SSL exceptions are discarded, except for http-over-https where we send back an http error response. For a subset of those exceptions we silient discard them, and for others we will print out the stack trace. This patch updates the code to silient discard all exceptions unless it is the http-over-https case.
This patch turns a new `ssl.SSLEOFError` into an internally ignored `FatalSSLAlert` allowing it not to leak into the outer abstraction layers in its raw form. The exception is new since Python 3.8 and it's fine to use it unconditionally since we no longer support Python 3.7. This patch also handles `SSLZeroReturnError` same as `SSLEOFError` as it's semantically equivalent per [[1]]. [1]: cherrypy#518 (comment)
Specifically, this patch adds an exception interception code to the place where the socket is being first wrapped. In case of the `builtin` TLS adapter, this is also a place where a handshake attempt happens. This switches the method of relaying an unrecoverable connection error from a sentinel of a tuple with two falsy values to raising a unified exception consistently.
This patch extends the processing of a case when a client attempts sending plain HTTP into an HTTPS port by emitting a log message.
|
Thanks @toppk! |
with python 3.8 and above ssl will generate SSLZeroReturnError exceptions for zero-bytes-send connections. These connections are ignored in cheroot when the exception is SSLError with errno=ssl.SSL_ERROR_EOF
this is the cpython commit that introduced the change of behavior.
python/cpython#18772
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #517
❓ What is the current behavior? (You can also link to an open issue here)
Annoying stack trace on startup, (see issue for full details), but cherrypy still operates normally
❓ What is the new behavior (if this is a feature change)?
No exception is printed, no other change in behavior.
📋 Other information:
📋 Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences
This change is